Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Cooldown Maintenance] Added More Spells #46

Closed
wants to merge 3 commits into from

Conversation

DaleHuntGB
Copy link
Contributor

Added:

Added Essence Break (Havoc Demon Hunter)
Added Touch of the Magi (Arcane Mage)
@Tercioo
Copy link
Owner

Tercioo commented Dec 18, 2023

I might be wrong here, but I think the cooldown time threshold to include cooldowns of type 1 (attack cooldowns) is 60 seconds.
Could you check the list and confirm this?

@DaleHuntGB
Copy link
Contributor Author

DaleHuntGB commented Dec 18, 2023

Okay, after some more testing...

Touch of the Magi shows correctly until CDR is used (for example, Shifting Power), the update in time then works but the ability automatically hides afterwards - as you can see here.

Essence Break shows correctly once being used but conditions are not updated once the spell has returned off cooldown. I am using Echo Raid Tools, a simple desaturation is added when the spell is on cooldown and then the desaturation is removed on the ability being available to use but this seems to not be the case on Essence Break.

Happy to wait for this merge, as more spells are not working. Would reducing the minimum cooldown be possible?

TL;DR - Minimum CD Threshold for Attack Cooldowns is definitely 60.

- Bonedust Brew [Already existed].

Updated:
- Shifting Power SpellID.
@Tercioo
Copy link
Owner

Tercioo commented Dec 20, 2023

Happy to wait for this merge, as more spells are not working. Would reducing the minimum cooldown be possible?

The questions for this issue are:

  • When cooldowns below 1 minute are required to track? Not on raid and m+, perhaps arenas?
  • Adding down to 45 seconds, would increase the amount of cooldowns added and with that, the data (in bytes) that is required to transmit to other players, would also increase.

@DaleHuntGB
Copy link
Contributor Author

DaleHuntGB commented Dec 21, 2023

When cooldowns below 1 minute are required to track? Not on raid and m+, perhaps arenas?

Definitely for arenas, if people find that useful but I would assume most people are looking to mimic the functionality that is provided by OmniCD but might prefer Echo Raid Tools (as I do).

Adding down to 45 seconds, would increase the amount of cooldowns added and with that, the data (in bytes) that is required to transmit to other players, would also increase.

Completely understand this, and ultimately, it won't be for every use case, I tend to only track the main offensive cooldowns (2mins or more) but Touch of the Magi is a pretty important cooldown as far as burst goes, so that might be useful to track.

@Tercioo
Copy link
Owner

Tercioo commented Jan 6, 2024

Hi, found a solution.
Could you add the key arena = true on cooldowns only available for pvp arena?
I don't know if your current comment has pvp cooldown or it is ready to merge, please let me know.

@DaleHuntGB
Copy link
Contributor Author

I am happy to do that but out of interest, how does this solve the CD durations between less than a minute? Won't tracking still be an issue?

@Tercioo
Copy link
Owner

Tercioo commented Jan 7, 2024

Hi!
Regular cooldowns with and above 1 minute, will still be sharing as usual.
But, inside an arena, it also shares all cooldowns marked as arena = true.

@DaleHuntGB
Copy link
Contributor Author

Sorry for not updating this. I will do some testing soon (university is manic).

Just out of interest, how does tracking work for Blessing of Freedom, this is a 25s CD but works effectively for tracking.

There are a few spells that I'd like to add but the CD limit being 1min is causing some issues, I appreciate this should be fixed for arena = true check, but this would not count for Dungeon/Raid content?

@Tercioo
Copy link
Owner

Tercioo commented Nov 6, 2024

Communication on this pr has been made with smoke signals :D
Closing has the expansion moved on.

@Tercioo Tercioo closed this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants